Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Вильданов Савелий #218

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Saveliy21
Copy link

alternativeSerializeProperties[propertyInfo] = o => serializeFunc((T) o);
}

public void AddLengthOfProperty(PropertyInfo propertyInfo, int length)
Copy link

@SlavikGh0st SlavikGh0st Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Этот метод не должен быть доступен для потребителя

ObjectPrinter.For<Person>().AddLengthOfProperty();  //не должно быть доступа 

return propertyInfo;
}

public void AddSerializedProperty<T>(PropertyInfo propertyInfo, Func<T, string> serializeFunc)
Copy link

@SlavikGh0st SlavikGh0st Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Этот метод не должен быть доступен для потребителя

ObjectPrinter.For<Person>().AddSerializedProperty();  //не должно быть доступа 

return this;
}

public PrintingConfig<TOwner> SerializeTypeWithSpecial<T>(Func<object, string> serializeFunc)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. По заданию подразумевалось что вызов этого метода будет после выбора типа или свойства:
ObjectPrinter.For<Person>()
   .For<int>().Serialize(...)
   ...
   
ObjectPrinter.For<Person>()
   .For(x => x.Age).Serialize(...)
   ...
  1. У нас вроде generic-метод, а почему в принимаем функцию от object?

return this;
}

public PrintingConfig<TOwner> SelectCulture<T>(CultureInfo cultureInfo)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. По заданию: "Для всех типов, имеющих культуру, есть возможность ее указать".
  2. По примеру из кода подразумевался такой вызов:
ObjectPrinter.For<Person>()
   .For<double>().UseCulture(...)
   ...

public class PropertyPrintingConfig<TOwner, TPropType>
{
public readonly PrintingConfig<TOwner> PrintingConfig;
public readonly PropertyInfo PropertyInfo;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

К этим полям не должно быть доступа для потребителя

ObjectPrinter.For<Person>().SelectField(x => x.Id).PrintingConfig; //не должно быть доступа 
ObjectPrinter.For<Person>().SelectField(x => x.Id).PropertyInfo; //не должно быть доступа 

@@ -14,28 +33,160 @@ public string PrintToString(TOwner obj)
private string PrintToString(object obj, int nestingLevel)
{
//TODO apply configurations
var identation = new string('\t', nestingLevel + 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Очепятка: indentation

}


private string CheckSerializationConditions(object obj, string identation)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно пометить, что object может быть nullable символом ?:

private string CheckSerializationConditions(object? obj, string identation)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Что-то не так с методом:

  1. Параметр identation не используется.
  2. Называется метод "Проверь условия сериализации", но при этом он возвращает сериализованную строку, а в каких-то случаях null.

if (obj == null)
return "null" + Environment.NewLine;
if (excludedTypes.Contains(obj.GetType()))
return "";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return string.Empty;

@@ -14,28 +33,160 @@ public string PrintToString(TOwner obj)
private string PrintToString(object obj, int nestingLevel)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Что-то не так с алгоритмом: у нас есть метод CheckSerializationConditions и логика в нём частично повторяется тут. Мб как-то можно более проще сделать?

sb.Append(identation + propertyInfo.Name + " = " +
propertyFunc(propertyInfo.GetValue(obj)) + Environment.NewLine);
else
sb.Append(identation + propertyInfo.Name + " = " +

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если у тебя много переносов, значит что-то не чисто:

  1. Не экономь на переменных - это бесплатно:
var serializedObj = ...
sb.Append(serializedObj);
  1. Не гонись за "действиями в одну строку" - если потребуется в условную конструкцию добавить скобки - ничего страшного.


namespace ObjectPrinting.Tests
{
public class ObjectPrinterTests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

По-хорошему тесты в отдельный проект выносят - но на это могу в целом закрыть глаза.
В следующих домашках уже буду требовать.

private Person person;

[SetUp]
public void Setup()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не вижу смысла в этом SetUp: у тебя статический объект, который не меняется - его можно вынести в поля класса:

private static readonly Person Person = new()
            { Name = "Monkey", SecondName = "D.Luffy", NameOfPet = "Usopp", Height = 1234567.89, Age = 17 };

}

[Test]
public void ObjetctPrinter_ShouldCorrectPrint_WithOutExcludingType()
Copy link

@SlavikGh0st SlavikGh0st Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему ...Without...?
Тут же как раз с ним - логичнее ObjetctPrinter_ShouldCorrectPrint_WithExcludingType.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поправь и в других тестах.

result.Should().NotContain(person.Name)
.And.NotContain(person.SecondName)
.And.NotContain(person.NameOfPet);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проблемы всех тестов:

  1. Не соблюдается паттерн ААА, все тесты должны выглядеть так:
{
   //блок подготовки данных
   var person = ...
   var printer = ....
   var expected = ...
  
  //блок действия
  var actual = ...
  
  //блок проверки данных
  actual.Should(). ...
}
  1. Используй var - пускай машина думает.

var printer = ObjectPrinter.For<Person>()
.SelectCulture<double>(new CultureInfo("fr-FR"));
string result = printer.PrintToString(person);
result.Should().Contain("1\u00a0234\u00a0567,890");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай тут попроще какие-нить кейсы)

  1. Проверка, что при одной культуре у нас double сериализуется с точкой "1.1".
  2. Проверка, что при другой культуре у нас double сериализуется с запятой "1,1".

var printerRu = ObjectPrinter.For<Person>()
.Printing<double>().Using(new CultureInfo("ru-Ru"));

printerEn.PrintToString(Person).Should().Contain("168.8");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Раздели это на 2 тест-кейса через атрибут [TestCase]

{Name = "Monkey", SecondName = "D.Luffy", NameOfPet = "Usopp", Height = 168.8, Age = 17};

[Test]
public void ObjetctPrinter_ShouldCorrectPrint_WithExcludingType()
Copy link

@SlavikGh0st SlavikGh0st Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. В наименовании всех тестов опечатка Objetct
  2. Во всех тестах блок проверки перемешался с блоком действия.

}

[Test]
public void ObjetctPrinter_ShouldCorrectPrint_WithDictionary()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Во всех тестах на коллекцию разделить на блоки AAA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants